Improve take performance on List arrays#9643
Conversation
|
Results on the benchmarks in #9626: |
8b7e3be to
c66bad6
Compare
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
c66bad6 to
95d54ac
Compare
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/list-take-perf-improvement (95d54ac) to aac969d (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
🚀 |
| } | ||
| } | ||
| Some(output_nulls) => { | ||
| new_offsets.resize(indices.len() + 1, OffsetType::Native::zero()); |
There was a problem hiding this comment.
Why initialize the offsets to zero, when they are immediately overwritten?
You could probably use push and extend instead of fill and setting offsets directly
There was a problem hiding this comment.
I assumed that this would be a nice way to avoid unsafe while keeping performance, but locally perf seems to be as good
| ) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
It would make it easier to see what you have changed if you didn't also move the tests around
There was a problem hiding this comment.
no idea why I did that
|
I'll address all the comments later today, should be ready by tomorrow |
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/list-take-perf-improvement (57df7c6) to aac969d (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
🥳 |
|
Thanks @AdamGS |
Which issue does this PR close?
Rationale for this change
This PR builds on top of #9626, improving the results on those benchmarks.
What changes are included in this PR?
take_bytesperf in the null cases between 10-25% #9625, branch the function into the null and non-null pathsAre these changes tested?
Added a few tests for sliced list arrays.
Are there any user-facing changes?
No